fix(x/tx): fix amino json drift from legacy spec #21825
Conversation
📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 WalkthroughWalkthroughThe pull request includes changes across multiple files, focusing on refining type handling, serialization, and testing frameworks within the Cosmos SDK. Key modifications involve removing certain type declarations, enhancing error handling in encoding functions, and introducing a new testing fixture to streamline test setups. Additionally, the changes aim to improve the organization and maintainability of the codebase by modularizing components and optimizing existing logic. Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
kocubinski
requested review from
testinginprod,
aaronc and
a team
as code owners
This comment has been minimized.
This comment has been minimized.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Outside diff range and nitpick comments (2)
tests/integration/tx/internal/util.go (1)
210-210: Typographical Correction in CommentIn the comment on line 210, there is a missing word. It should read: "It is used here to ensure the x/tx JSON encoding..."
Apply this diff to fix the typo:
// amino JSON encoding x/auth/migrations/legacytx.StdSignBytes. It is used here ensure the x/tx +// amino JSON encoding x/auth/migrations/legacytx.StdSignBytes. It is used here to ensure the x/txtests/integration/tx/aminojson/aminojson_test.go (1)
145-146: Maintain consistent formatting for module listings in fixture initialization.In
TestAminoJSON_LegacyParity, the modules are listed in a compact format, whereas inTestAminoJSON_Equivalence, each module is listed on a separate line. For consistency and improved readability, consider formatting the module list in the same way in both functions.
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (5)
- tests/integration/rapidgen/rapidgen.go (1 hunks)
- tests/integration/tx/aminojson/aminojson_test.go (4 hunks)
- tests/integration/tx/context_test.go (2 hunks)
- tests/integration/tx/internal/util.go (1 hunks)
- x/tx/signing/aminojson/encoder.go (4 hunks)
Additional context used
Path-based instructions (5)
tests/integration/rapidgen/rapidgen.go (2)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
tests/**/*: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"tests/integration/tx/aminojson/aminojson_test.go (3)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
tests/**/*: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"tests/integration/tx/context_test.go (3)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
tests/**/*: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"tests/integration/tx/internal/util.go (2)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
tests/**/*: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"x/tx/signing/aminojson/encoder.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Additional comments not posted (15)
tests/integration/tx/context_test.go (3)
10-10: LGTM!Moving the custom signer logic to a separate internal package is a good refactoring decision. It improves code organization and maintainability.
14-14: LGTM!Importing the
requirepackage fromtestifyis a standard practice for writing assertions in Go tests. It will help in validating the expected behavior of the code under test.
29-29: LGTM!Providing the
internal.ProvideCustomGetSignerfunction to the dependency injection container is the correct way to make the custom signer logic available for the tests after the refactoring. It ensures that the tests can access the custom signer functionality.tests/integration/rapidgen/rapidgen.go (2)
262-264: Skipping review of the comment.The comment provides useful context about a pending fix in a separate pull request. The TODO comment is a good practice to track pending tasks. No changes are required for this comment in this pull request.
262-264: Skipping review of the commented out code.The code segment is intentionally commented out due to a pending fix in a separate pull request. Commenting out the code is a good practice to avoid breaking changes until the fix is merged. No changes are required for this code segment in this pull request.
x/tx/signing/aminojson/encoder.go (9)
56-61: Correctly handle decimal unmarshalling incosmosDecEncoder.The code properly unmarshals the decimal value using
dec.Unmarshal([]byte(val))and marshals it back to a string representation. This ensures that decimal values are accurately processed and enhances the robustness of the encoder.
136-140: Fields added tomoduleAccountPrettyare consistent with JSON output.The inclusion of
AddressandPubKeyfields with appropriate JSON tags ensures that all necessary account information is included in the serialized JSON output. This aligns with the expected structure for module accounts.
146-152: Proper type assertion inmoduleAccountEncoder.The code correctly asserts that the provided
msgis of typeauthapi.ModuleAccountby comparing the full names of the descriptors. This improves type safety and prevents potential runtime errors due to incorrect message types.
154-156: InitializePubKeyand populateNameinmoduleAccountPretty.The
PubKeyfield is initialized as an empty string, and theNamefield is correctly retrieved from the message fields. This ensures that the JSON output includes these fields, even if the public key is not set.
162-168: Extract base account information if available.The code correctly checks for the presence of
base_accountand extractsAddress,AccountNumber, andSequencefields. This enhances flexibility and prevents potential nil pointer dereferences when the base account is not set.
189-191: Initialize variables inthresholdStringEncoder.The necessary variables
pk,msgDesc, andfieldsare properly initialized to process the multisig public key message. This sets the foundation for accurate serialization of multisig keys.
199-201: Write opening of JSON object and handle errors properly.The code correctly writes the beginning of the JSON object with
{"pubkeys":and checks for potential I/O errors. This ensures that any write failures are promptly handled.
203-213: Ensure correct JSON serialization ofpubkeys.The function properly handles the case where
pubkeysis empty by writing an empty array[]. Whenpubkeysare present, it serializes the list usingenc.marshalList, ensuring accurate JSON representation.
215-216: Serializethresholdfield as a string in JSON output.The code appends the
thresholdvalue as a string to the JSON output usingfmt.Fprintf. Serializing numeric values as strings maintains compatibility with legacy amino JSON encoding requirements.tests/integration/tx/aminojson/aminojson_test.go (1)
99-99: Uset.Logfinstead offmt.Printffor test logging.In test code, it's recommended to use
t.Logffor logging instead offmt.Printf. This ensures that log messages are properly associated with the test and can be filtered or formatted by the testing framework.Apply this diff to make the change:
- fmt.Printf("testing %s\n", tt.Pulsar.ProtoReflect().Descriptor().FullName()) + t.Logf("testing %s", tt.Pulsar.ProtoReflect().Descriptor().FullName())Likely invalid or redundant comment.
| testMsg := msg.(*testpb.TestRepeatedFields) | ||
| // arbitrary logic | ||
| signer := testMsg.NullableDontOmitempty[1].Value | ||
| return [][]byte{[]byte(signer)}, nil |
There was a problem hiding this comment.
Prevent Potential Index Out of Range Panic in ProvideCustomGetSigner
In the Fn function (lines 35-40), accessing testMsg.NullableDontOmitempty[1] at line 38 without checking the slice length may cause an index out of range panic if the slice has fewer than two elements. To ensure robustness, add a check to confirm that testMsg.NullableDontOmitempty has at least two elements before accessing.
Apply this diff to add the safety check:
func (msg proto.Message) ([][]byte, error) {
testMsg := msg.(*testpb.TestRepeatedFields)
+ if len(testMsg.NullableDontOmitempty) <= 1 {
+ return nil, fmt.Errorf("NullableDontOmitempty must have at least two elements")
+ }
// arbitrary logic
signer := testMsg.NullableDontOmitempty[1].Value
return [][]byte{[]byte(signer)}, nil
}Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| testMsg := msg.(*testpb.TestRepeatedFields) | |
| // arbitrary logic | |
| signer := testMsg.NullableDontOmitempty[1].Value | |
| return [][]byte{[]byte(signer)}, nil | |
| testMsg := msg.(*testpb.TestRepeatedFields) | |
| if len(testMsg.NullableDontOmitempty) <= 1 { | |
| return nil, fmt.Errorf("NullableDontOmitempty must have at least two elements") | |
| } | |
| // arbitrary logic | |
| signer := testMsg.NullableDontOmitempty[1].Value | |
| return [][]byte{[]byte(signer)}, nil |
| permissions := msg.Get(fields.ByName("permissions")).List() | ||
| for i := 0; i < permissions.Len(); i++ { | ||
| pretty.Permissions = append(pretty.Permissions, permissions.Get(i).String()) | ||
| } |
There was a problem hiding this comment.
Preallocate Permissions slice to improve performance.
Since the length of permissions is known, consider preallocating the Permissions slice to avoid multiple allocations during the append operations.
Apply this diff to preallocate the slice:
permissions := msg.Get(fields.ByName("permissions")).List()
+pretty.Permissions = make([]string, 0, permissions.Len())
for i := 0; i < permissions.Len(); i++ {
pretty.Permissions = append(pretty.Permissions, permissions.Get(i).String())
}Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| permissions := msg.Get(fields.ByName("permissions")).List() | |
| for i := 0; i < permissions.Len(); i++ { | |
| pretty.Permissions = append(pretty.Permissions, permissions.Get(i).String()) | |
| } | |
| permissions := msg.Get(fields.ByName("permissions")).List() | |
| pretty.Permissions = make([]string, 0, permissions.Len()) | |
| for i := 0; i < permissions.Len(); i++ { | |
| pretty.Permissions = append(pretty.Permissions, permissions.Get(i).String()) | |
| } |
…into kocu/amino-json-tests
julienrbrt
added
the
backport/v0.50.x
|
adding backport label to 0.50 to release 0.13.x. |
|
oh I need to add the backport 0.52 label as well to backport the testing suite changes. |
julienrbrt
added
the
backport/v0.52.x
(cherry picked from commit 2d40cc1) # Conflicts: # tests/integration/tx/aminojson/aminojson_test.go # x/tx/CHANGELOG.md # x/tx/signing/aminojson/encoder.go
(cherry picked from commit 2d40cc1) # Conflicts: # tests/integration/tx/aminojson/aminojson_test.go # x/auth/migrations/legacytx/stdtx_test.go # x/tx/CHANGELOG.md
(cherry picked from commit 2d40cc1) fix half conflicts and revert tests conflicts
(cherry picked from commit 2d40cc1) fix half conflicts and revert tests conflicts
* reset to upstream * feat(x/tx): support bytes field as signer (cosmos#21850) (cherry picked from commit dbf6cdb) * fix(x/tx): fix amino json drift from legacy spec (cosmos#21825) (cherry picked from commit 2d40cc1) fix half conflicts and revert tests conflicts * changelog
Description
Related to: #21782
Recent changes had caused all amino JSON legacy equivalence rapid tests to early out before running. As a consequence changes to the x/tx Amino JSON encoder were not properly tested, and several issues were not caught.
They are:
The tests and specs for amino JSON encoding have been tightened up. The tests now closely mimic the same code paths followed in the updated TxDecoder in x/tx.
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!in the type prefix if API or client breaking changeCHANGELOG.mdReviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
Please see Pull Request Reviewer section in the contributing guide for more information on how to review a pull request.
I have...
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Chores
x/txmodule regarding recent fixes.